Skip to content

HermitCrab: keep final-template pruning on when only lexical entries are partial - #553

Open
johnml1135 wants to merge 3 commits into
masterfrom
fix-551-partial-entry-prune
Open

johnml1135 wants to merge 3 commits into
masterfrom
fix-551-partial-entry-prune

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #551.

Problem

The final-template prune from #491 is skipped whenever Morpher.IsPartial is true, and IsPartial counts partial lexical entries. A grammar with partial entries and no partial rules never prunes.

Why entries can be ignored

#551 suspected this was unsound: a word inherits partiality from its root (Word.cs:150), and synthesis lets a non-partial rule follow a final template when the input word is partial (SynthesisAffixProcessRule.cs:67). That would let a partial root license a derivation the prune rejects.

It does not, because SynthesisAffixTemplatesRule skips every template when input.RootAllomorph.Morpheme.IsPartial. No template, final or not, applies to a word whose root is partial. One nuance: SynthesisAffixTemplatesRule.Apply calls ChooseInflectionalStem before that check, so a partial root with a non-partial LexFamily relative can still take a template, with the relative as the root. The prune stays sound because that word is non-partial, so a following non-partial rule is blocked either way.

Partial rules are different. A partial rule makes the word partial after the root, and the template can still apply, so a non-partial rule can then follow a final template. They keep disabling the prune.

Change

  • Morpher.HasPartialRules: true if any partial morpheme is not a LexEntry. IsPartial is removed; nothing reads it now.
  • AnalysisAffixTemplateRule and AnalysisAffixProcessRule read HasPartialRules instead of IsPartial.
  • AffixTemplateTests.PartialLexicalEntryDoesNotDisableEarlyPruningOfFinalTemplate: with a partial entry and no partial rule the prune fires (trace depth 2) and a partial root parses the same as before (sab, sabv parse; sabd, sabdv do not). With a partial rule the prune stays off and sagdv still parses as 32 PAST NOM. The test fails with the old guard (the prune is reached at depth 6 instead of 2).

Evidence

  • Fixture conformance/edge-cases/final-template-partial-root on Add a conformance suite for HermitCrab, with its adequacy argument #480 (integrate-conformance-framework at fa8d9b2) has partial roots in a lower stratum and in the template's own stratum. Every final-template string on a partial root has no analysis, with non-partial controls.
  • In a scratch copy with the same guard, all 142 conformance runs over 132 fixtures (including pathological) passed, and the prune fired on the new fixture.
  • ./local_check.sh --agent-strict passed on this branch before it was rebased onto master. After the rebase and the IsPartial removal, only AffixTemplateTests and PruneDisagreeingCopiesTests were rerun (46/46).

Not covered

Inflectional-family stem substitution (ChooseInflectionalStem) replaces the root with a family relative before the template check. I traced it by reading only (see the nuance above). No fixture exercises it.

Lexical-pattern guesses were already excluded from the partial-morpheme set and pruned.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.73%. Comparing base (c245476) to head (b507a89).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage   74.57%   74.73%   +0.16%     
==========================================
  Files         453      453              
  Lines       38342    38342              
  Branches     5284     5284              
==========================================
+ Hits        28593    28656      +63     
+ Misses       8583     8532      -51     
+ Partials     1166     1154      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/SIL.Machine.Morphology.HermitCrab/Morpher.cs Outdated
Comment thread tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs Outdated
@claude

This comment has been minimized.

@johnml1135
johnml1135 force-pushed the fix-551-partial-entry-prune branch from 57a3b7e to f1ac77e Compare October 26, 2026 11:49
johnml1135 added a commit that referenced this pull request Oct 26, 2026
PR #553 shows ignoring partial lexical entries in the final-template prune
guard is safe, so Sena gets the prune once it merges.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
/// template that follows a non-template rule only when this is false. Partial lexical entries do not
/// count: <see cref="SynthesisAffixTemplatesRule"/> applies no template to a word whose root is partial,
/// and when it substitutes an inflectional-family relative for the root, the relative's word is
/// non-partial, so a following non-partial rule is blocked either way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F3. The summary's reasoning leaves out blocking. A blockable template slot rule can call Word.CheckBlocking (SynthesisAffixProcessRule.cs:199), which swaps in a partial family relative. SynthesisAffixTemplatesRule.cs:45 then marks that word final, and a non-partial rule may follow it because input.IsPartial (SynthesisAffixProcessRule.cs:67). The output matches the relative analyzed directly as a root, so I expect no parse is lost. That is Unverified. Consider cutting the summary to its first sentence.

@claude

This comment has been minimized.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

I think that you can remove Morpher.IsPartial in the code and in the comments, since it is superceded by Morpher.HasPartialRules.

johnml1135 and others added 3 commits October 27, 2026 09:56
The analysis-side final-template prune (491) was switched off whenever Morpher.IsPartial
was true, and IsPartial counts partial lexical entries. Grammars with partial entries but
no partial rules therefore never pruned.

A partial entry cannot make the prune lose a parse. A word takes its partiality from its
root, but SynthesisAffixTemplatesRule applies no template to a word whose root morpheme is
partial, so a derivation that unapplies a final template can never end at a partial root.
Partial rules differ: they make a word partial after the template has applied, which lets
a non-partial rule follow the final template, so they still disable the prune.

Add Morpher.HasPartialRules and read it in the two analysis guards. IsPartial keeps its
meaning. Lexical-pattern guesses were already excluded from IsPartial and pruned.

Closes #551

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The summary claimed a derivation that unapplies a final template can never end
at a partial root. SynthesisAffixTemplatesRule.Apply calls ChooseInflectionalStem
before the root check, so a partial root can take a template through a
non-partial family relative. State the contract and why the prune stays sound.

The test left the partial "sab" entry in the shared Language; remove it in a
finally block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing reads it now that the analysis-side final-template checks use
HasPartialRules. The three test preconditions assert GetPartialMorphemes()
directly.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the fix-551-partial-entry-prune branch from f1ac77e to b507a89 Compare October 27, 2026 17:57
@johnml1135

Copy link
Copy Markdown
Collaborator Author

I think that you can remove Morpher.IsPartial in the code and in the comments, since it is superceded by Morpher.HasPartialRules.

Done in b507a89: I removed Morpher.IsPartial, its assignment and the stale comment in Morpher.cs. Nothing read it after the analysis-side checks moved to HasPartialRules. The three test preconditions that used it now assert GetPartialMorphemes() directly. csharpier passes, and AffixTemplateTests and PruneDisagreeingCopiesTests pass (46/46). The branch is also rebased onto master.

/// non-partial, so a following non-partial rule is blocked either way.
/// </summary>
public bool IsPartial { get; }
public bool HasPartialRules { get; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F4. Removing Morpher.IsPartial is a breaking public API change. It shipped in v3.9.5 and v3.9.6 (added in #491), so a consumer that reads it fails to compile, or throws MissingMethodException on a drop-in upgrade. I know this was requested. If no consumer such as FieldWorks reads it, nothing more is needed. Otherwise keep it as an [Obsolete] property for one release. Unverified: I did not search FieldWorks.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FieldWorks doesn't read it. I think that nothing further needs to be done.

var morpher = new Morpher(TraceManager, Language);
Assert.That(morpher.IsPartial, Is.True);
Assert.That(morpher.GetPartialMorphemes(), Is.Not.Empty);
morpher.AlwaysEnforceFinalTemplates = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F5. No test covers the analysis-side || _morpher.AlwaysEnforceFinalTemplates override any more. The base grammar's only partial morphemes are lexical entries, so HasPartialRules is false here and at lines 697-743 and 860, and the flag has no effect on analysis. Your new test shows the same depth-2 prune at line 547 with the flag off. A partial rule with the flag on (for example, at the end of the new test) would cover it.

Is.Not.Empty,
"entry 54 has an empty syntactic feature structure"
);
morpher.AlwaysEnforceFinalTemplates = alwaysEnforceFinalTemplates;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F6. The two TestCases now run the same path. Entry 54 is a LexEntry, so HasPartialRules is false, and analysis prunes final templates whether alwaysEnforceFinalTemplates is true or false. Copy-pruning with the final-template prune off is no longer tested. A partial rule in the fixture would restore the false case.

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. Removing IsPartial changes no behavior: nothing in the library read it once the guards moved to HasPartialRules.
  2. Most important: F4, src/SIL.Machine.Morphology.HermitCrab/Morpher.cs:107. The removed Morpher.IsPartial shipped in v3.9.5 and v3.9.6, so removing it breaks any consumer that reads it.
  3. Counts: 0 Critical, 0 Important, 3 Low new (4 Low open in total).
  4. Ran: dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests --filter "FullyQualifiedName~AffixTemplateTests|FullyQualifiedName~PruneDisagreeingCopiesTests", which passed 46 of 46. gh pr checks 553: Build on ubuntu-22.04 and windows-latest pass, Create NuGet package pass, Comment hygiene pass (advisory), claude-review pending. History was rewritten since f1ac77e, so I reviewed the whole PR again.
  5. Not verified: whether FieldWorks or another consumer reads Morpher.IsPartial (F4), and that the CheckBlocking path in F3 loses no parse.

Public API: Morpher.IsPartial is removed and Morpher.HasPartialRules is added. Analysis now runs the final-template prune for grammars whose only partial morphemes are lexical entries. Target framework, package, and sillsdev/machine.py parity: none verified.

Findings: F1 addressed, F2 addressed, F3 open (unverified), F4 new (unverified), F5 new, F6 new.

Reviewed at b507a89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HermitCrab: partial lexical entries switch off final-template pruning for the whole grammar

3 participants